fix: preserve webhook URL query params in generic notification provider (#2292)#2345
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| query := webhookURL.Query() | ||
|
|
||
| // Set template to JSON (default for generic webhooks). Shoutrrr's JSON | ||
| // template marshals the notification params as a flat JSON object at the | ||
| // root level, which is the format most providers (PushPlus, custom APIs, | ||
| // Home Assistant, etc.) expect. | ||
| query.Set("template", "json") |
There was a problem hiding this comment.
Silent overwrite of user-provided Shoutrrr config keys
The code seeds query from the user's webhook URL and then unconditionally calls query.Set("template", "json"). If a user's webhook URL already contains a key that Shoutrrr interprets as a config key (template, disabletls, method, contenttype, titlekey, messagekey), their value is silently discarded. The most likely collision is template — any user who deliberately places ?template=... in their webhook URL will always have it overwritten to json. Same applies if the URL contains ?disabletls=..., which is always overwritten by the scheme-derived value.
Consider documenting the known collision behavior in a comment, or — where the config field is unset — skipping query.Set when the key is already present via query.Get:
if query.Get("template") == "" {
query.Set("template", "json")
}This would preserve an explicit user override while still providing the default for the common case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/pkg/utils/notifications/generic_sender.go
Line: 51-57
Comment:
**Silent overwrite of user-provided Shoutrrr config keys**
The code seeds `query` from the user's webhook URL and then unconditionally calls `query.Set("template", "json")`. If a user's webhook URL already contains a key that Shoutrrr interprets as a config key (`template`, `disabletls`, `method`, `contenttype`, `titlekey`, `messagekey`), their value is silently discarded. The most likely collision is `template` — any user who deliberately places `?template=...` in their webhook URL will always have it overwritten to `json`. Same applies if the URL contains `?disabletls=...`, which is always overwritten by the scheme-derived value.
Consider documenting the known collision behavior in a comment, or — where the config field is unset — skipping `query.Set` when the key is already present via `query.Get`:
```go
if query.Get("template") == "" {
query.Set("template", "json")
}
```
This would preserve an explicit user override while still providing the default for the common case.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Fixes #2292 — the Generic Webhook provider could not deliver to PushPlus (and any other endpoint that authenticates via a query-string token) because
BuildGenericURLencoded the user's?as%3Fand stuffed the original query into the path of the Shoutrrr service URL. Shoutrrr then forwarded that literal path verbatim, so the outbound HTTP request looked like:PushPlus replied
200 OK(because the host responds to anything under/) but thetokenwas sitting inside the path, not the query, so no notification was ever delivered.Root cause
backend/pkg/utils/notifications/generic_sender.gobuilt the Shoutrrr URL like this:When Go re-emits a URL with a
?insidePath,EscapedPath()re-encodes it to%3F, which meansRequestURI()produces/send%3Ftoken=...and the query is gone for good.Shoutrrr's generic service already preserves any query keys it doesn't recognise (verified upstream in
generic_test.go—extra=paramsurvivesConfigFromWebhookURL), so the%3Fworkaround was never needed.Fix
%3Fencoding entirelywebhookURL.Query()so the user's existing query parameters merge naturally with the Shoutrrr config keys (template,messagekey,method,disabletls, …)Path: webhookURL.Pathand the merged queryEnd result for the reporter's PushPlus configuration (
http://www.pushplus.plus/send?token=YOUR_TOKEN+messageKey=content):generic://www.pushplus.plus/send%3Ftoken=YOUR_TOKEN?...generic://www.pushplus.plus/send?token=YOUR_TOKEN&...RequestURI/send%3Ftoken=YOUR_TOKEN/send?token=YOUR_TOKEN{"title":"...","content":"..."}{"title":"...","content":"..."}The body was already a flat JSON at the root level — Shoutrrr's
template=jsonmarshalsparamsdirectly — so no changes were needed there. The reporter's diagnosis ("payload mismatch") was a symptom of the URL being wrong; the body becomes deliverable as soon as the token reaches PushPlus.Tests
BuildGenericURLcases that asserted the%3Fencoding to assert the new merged-query formPushPlus webhook with content message keycase that mirrors the reporter's exact configurationservice.Config.WebhookURL().RequestURI()resolves to/send?token=abc123,MessageKey=content,RequestMethod=POSTgo test ./backend/pkg/utils/notifications/...✅gofmt -lclean,go vetcleanTest plan
URL=http://www.pushplus.plus/send?token=YOUR_TOKEN,Method=POST,Message Key=content/api/webhook/<id>, custom JSON endpoints, etc.)Disclaimer Greptiles Reviews use AI, make sure to check over its work.
To better help train Greptile on our codebase, if the comment is useful and valid Like the comment, if its not helpful or invalid Dislike
To have Greptile Re-Review the changes, mention
greptileai.Greptile Summary
This PR fixes the Generic Webhook provider's handling of webhook URLs that include query-string authentication tokens by dropping the broken percent-encoding workaround and instead seeding Shoutrrr's query map directly from
webhookURL.Query(). The change is correct, the root cause is well-diagnosed, and the updated tests cover the reporter's exact configuration. The only minor concern is that Shoutrrr config keys (template,disabletls, etc.) will silently overwrite any identically-named query parameter a user places in their webhook URL — worth a brief comment if such collisions are possible in practice.Confidence Score: 5/5
Safe to merge — the fix is correct and the only remaining finding is a P2 style suggestion about silent key collisions.
All findings are P2 or lower. The core logic change is clearly correct: webhookURL.Query() properly seeds the Shoutrrr query map and url.URL with Path (not RawPath) produces the right RequestURI. Tests pass and cover the reporter's scenario.
No files require special attention.
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: preserve webhook URL query params i..." | Re-trigger Greptile